fix(reduce): fix overload ordering #2382

aluanhaddad · 2017-02-17T17:52:30Z

Description:
Type inference for Observable.prototype.reduce currently behaves incorrectly, resulting in spurious type errors, in many common cases, when the type of seed is a different type than the value type of the Observable.

One of the declared overloads is intended to specifically cater to this scenario but fails to do so because of its ordering with respect to the other overload declared.

The overload in question takes two type arguments, T and R, making it more specific than the overloads declared before it.

Due to TypeScript's overload resolution algorithm, which picks the first matching overload, this overload does not have the intended effect.

This is documented here https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html in the section entitled Function Overloads, under the Ordering heading, which states

Don’t put more general overloads before more specific overloads:

Do sort overloads by putting the more general signatures after more specific signatures:

Why: TypeScript chooses the first matching overload when resolving function calls. When an earlier overload is “more general” than a later one, the later one is effectively hidden and cannot be called.

Related issue (if exists):
This fixes #2338

Fixes #2338

kwonoj

Looks good to me, but would you mind add some test cases failing before and fixed by this PR?

aluanhaddad · 2017-02-17T18:28:40Z

@kwonoj Thank you, I most definitely will.

aluanhaddad · 2017-02-17T18:34:07Z

~~It looks like there is more nuance here, the overload taking an array seems to be producing more issues than I had thought.~~

Actually no it is just that the assumptions about where the type argument inference for seed actually comes from in the failing test cases seem incorrect.

rxjs-bot · 2017-02-17T19:23:21Z

	Warnings
⚠️	commit message does not follows conventional change log (1)

(1) : RxJS uses conventional change log to generate changelog automatically. It seems some of commit messages are not following those, please check contributing guideline and update commit messages.

Generated by 🚫 dangerJS

benlesh · 2017-02-20T23:37:11Z

@aluanhaddad it looks like there are build errors with the specs. Do you think you can fix them?

aluanhaddad · 2017-02-21T00:52:44Z

Yes I've been very busy recently. I will try to fix them tonight and add some additional tests as well. When I looked at the specs, they seemed to rely on the seed being inferred to have the value type of the Observable. I need to double-check if that is the case.
Perhaps I misunderstand the behavior of RxJS or the intent of the Declarations but this doesn't seem like a reasonable assumption at face value.

…iling specs This adds tests for both existing behavior, under the old overload, and the new behavior enabled by the new overload signatures of the `reduce` operator. Not that the specific change to the following test case `'should accept T typed reducers'` that removes the seed value. If a seed value is specified, then the reduction is always from `T` to `R`. This was necessary to make the test pass, but also models the expected behavior more correctly. The cases for `R` where `R` is not assignable to `T` are covered by new tests added in the commit. Additionally, I have added additional verification to all of the tests touched by this change to ensure that the values returned are usable as their respective expected types.

aluanhaddad · 2017-02-21T04:49:44Z

I am having some trouble replicating the test failures locally.
When I run

npm run build_spec && npm run test_mocha && node ./node_modules/markdown-doctest/bin/cmd.js

Any tips would be greatly appreciated.
node -v: 7.3.0
npm -v: 4.1.2
Windows 10 x64

Nevermind, I got it working.

Fix ordering as allowed by newly required seeds to fix failing tests. I think there is a good argument to be made that the failing tests were failing correctly, as this is how `Array.prototype.reduce` behaves, but as I have made the seed arguments required, for `R` typed reducers, re-ordering the overloads impacts their specificity allowing, the current behavior, correct or otherwise, to be retained.

coveralls · 2017-02-21T05:16:18Z

Coverage remained the same at 97.688% when pulling 99fd39d on aluanhaddad:patch-1 into 023d436 on ReactiveX:master.

aluanhaddad · 2017-02-21T05:23:48Z

@kwonoj I added several new test including tests covering behavior that was an error before these changes.
@Blesh I fixed the failing tests and also broke them down into multiple discrete tests, for assignability.

I added a local binding and a subsequent statement to each test I touched so that the resulting value is now used in such a way that it validates that the types are propagated.

Let me know if these tests are sufficient and also let me know if I should have added the same tests to spec/operator/scan-spec.

kwonoj · 2017-03-12T20:28:51Z

I think this change is ready, re-approving. /cc @david-driscoll for second eye just in case.

masaeedu · 2017-03-21T13:56:35Z

spec/operators/reduce-spec.ts

    type(() => {
      let a: Rx.Observable<{ a: number; b: string }>;
-      a.reduce<{ a?: number; b?: string }>((acc, value) => {
+      const reduced = a.reduce<{ a?: number; b?: string }>((acc, value) => {


Is a separate test required for generic type argument inference of R?

I believe so. Failure to infer R as distinct from T and T[] was the original issue.

@aluanhaddad This test doesn't seem to be verifying correct inference, because it specifies the generic type arg explicitly. Probably for an inference test you'd repeat this without the explicit type argument, then try to assign reduced to a variable of the explicit type you expect to have been inferred.

My bad, just realized the "R is not assignable to T" test already covers inference. 👍

masaeedu · 2017-03-21T13:56:49Z

lgtm

benlesh · 2017-03-21T23:13:17Z

@david-driscoll can you review this please? <3

david-driscoll

I have no idea how I missed this after being pinged several times... 👍

aluanhaddad · 2017-03-23T16:52:56Z

Just to note, when I first looked at this, I believed the issue was simply due to the overload ordering but the root cause turned out to have as much to do with the optionality of the seed arguments as signature ordering. The now required seed arguments eliminate a previously applicable overload from the candidate set, but a minor order change was still ultimately necessary.

benlesh · 2017-04-03T19:51:16Z

Thanks @aluanhaddad !!!

lock · 2018-06-06T12:40:53Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

fix(reduce): adjust overload ordering

7cee026

Fixes #2338

kwonoj requested a review from david-driscoll February 17, 2017 17:54

kwonoj reviewed Feb 17, 2017

View reviewed changes

Make seed required when not of type T

28b3ea9

kwonoj approved these changes Mar 12, 2017

View reviewed changes

kwonoj added the Next Patch Version label Mar 12, 2017

kwonoj mentioned this pull request Mar 21, 2017

Remove redundant overload signature #2484

Closed

masaeedu reviewed Mar 21, 2017

View reviewed changes

david-driscoll approved these changes Mar 21, 2017

View reviewed changes

benlesh merged commit f6a4951 into ReactiveX:master Apr 3, 2017

PrimalZed mentioned this pull request Apr 3, 2017

Reduce with array input defaults the return type to an array of arrays #2519

Closed

lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(reduce): fix overload ordering #2382

fix(reduce): fix overload ordering #2382

aluanhaddad commented Feb 17, 2017

kwonoj left a comment

aluanhaddad commented Feb 17, 2017

aluanhaddad commented Feb 17, 2017 •

edited

Loading

rxjs-bot commented Feb 17, 2017 •

edited

Loading

benlesh commented Feb 20, 2017

aluanhaddad commented Feb 21, 2017

aluanhaddad commented Feb 21, 2017 •

edited

Loading

coveralls commented Feb 21, 2017 •

edited

Loading

aluanhaddad commented Feb 21, 2017

kwonoj commented Mar 12, 2017

masaeedu Mar 21, 2017

aluanhaddad Mar 21, 2017

masaeedu Mar 21, 2017

masaeedu Mar 21, 2017

masaeedu commented Mar 21, 2017

benlesh commented Mar 21, 2017

david-driscoll left a comment

aluanhaddad commented Mar 23, 2017

benlesh commented Apr 3, 2017

lock bot commented Jun 6, 2018

fix(reduce): fix overload ordering #2382

fix(reduce): fix overload ordering #2382

Conversation

aluanhaddad commented Feb 17, 2017

kwonoj left a comment

Choose a reason for hiding this comment

aluanhaddad commented Feb 17, 2017

aluanhaddad commented Feb 17, 2017 • edited Loading

rxjs-bot commented Feb 17, 2017 • edited Loading

benlesh commented Feb 20, 2017

aluanhaddad commented Feb 21, 2017

aluanhaddad commented Feb 21, 2017 • edited Loading

coveralls commented Feb 21, 2017 • edited Loading

aluanhaddad commented Feb 21, 2017

kwonoj commented Mar 12, 2017

masaeedu Mar 21, 2017

Choose a reason for hiding this comment

aluanhaddad Mar 21, 2017

Choose a reason for hiding this comment

masaeedu Mar 21, 2017

Choose a reason for hiding this comment

masaeedu Mar 21, 2017

Choose a reason for hiding this comment

masaeedu commented Mar 21, 2017

benlesh commented Mar 21, 2017

david-driscoll left a comment

Choose a reason for hiding this comment

aluanhaddad commented Mar 23, 2017

benlesh commented Apr 3, 2017

lock bot commented Jun 6, 2018

aluanhaddad commented Feb 17, 2017 •

edited

Loading

rxjs-bot commented Feb 17, 2017 •

edited

Loading

aluanhaddad commented Feb 21, 2017 •

edited

Loading

coveralls commented Feb 21, 2017 •

edited

Loading